Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Node] add StmtsIterable interface to mark nodes that contain iterable stmts to improve hooking in node visitors #836

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

TomasVotruba
Copy link
Contributor

@TomasVotruba TomasVotruba commented May 15, 2022

At the moment, there are nodes in php-parser, that include list of other stmts. While iterating over can be easy with
property_exists($node, 'stmts') (feels like poor coding), the hooking to them via node visitors/Rector/PHPStan not so.

At the moment hooking into them requries special constant and careful listing of all such nodes:
https://github.com/rectorphp/rector-src/blob/975fdf113fab99b6120383211e997da2c820bd0a/rules/CodeQuality/NodeTypeGroup.php
It happened we forgot some and then Rector rules were not applied.

These rules often work with previously used stmt, e.g. to remove already assigned value:

 function setDefault()
 {
-    $value = 100;
     $value = 150;
     // ...
 }

This could be somehow achieved with next/previous/parent attributes. Yet PHPStan and Rector are moving away from next/previous/parent nodes for 2 reasons:

  • it's heavy for memory
  • it can take time to find firs previous stmts, as e.g. Assign and other Expr can be deeply nested:
if ($number === 1000 && ($value = 100)++ > --$breakpoint) {
    
}

Here removing $value = 100 itself is ilegal and requires quite complex check to all parent nodes.
Iterating only over all directly available stmts would make this no-brainer.


Where would be this interface be useful?

  • Rector could hook into all those nodes via single type
  • PHPStan that can hook only into single node (see getNodeType()) would be able to hook into those too
  • not just these 2 projects, but any php-parser based traverser could detect such node type easily

This marker will dramatically simplify integration of new rules to work with stmts.


At first I thought we could have class StmtsAware extends Stmt {}, but sometime Expr contains $stmts too:

/** @var Node\Stmt[] Statements */
public $stmts;

Thus interface marker choice. We're already using such marker interface in Rector via patching of php-parser nodes: https://github.com/rectorphp/rector/blob/d08b83c4264ba5b265890a049285518f79684644/vendor/nikic/php-parser/lib/PhpParser/Node/Stmt/Foreach_.php#L8

And it works great 👍


What do you think?

@TomasVotruba TomasVotruba requested a review from nikic June 8, 2022 19:31
@TomasVotruba
Copy link
Contributor Author

I feel like this got stuck but I don't know why. I'd love to get it before version 5 gets released.
What needs to be done here to finish it?

@nikic
Copy link
Owner

nikic commented Sep 21, 2022

I feel like this got stuck but I don't know why. I'd love to get it before version 5 gets released. What needs to be done here to finish it?

This is blocked on the nullable $stmts in ClassMethod. I'm not sure what to do about it :/

@TomasVotruba TomasVotruba force-pushed the tv-stmts-iterable branch 3 times, most recently from a60ca40 to 4602e8b Compare September 25, 2022 14:31
@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented Sep 25, 2022

I've removed the interface from ClassMethod, as the use case is really different. I also resolved the rest of the comments.


The main goal of this interface is to catch all the nested structures with single node, and it does well now 👍

if (...) {
    foreach (...) {
        $closure = function() {
            $item = 1;

            if (...) {
                return true;
            }
        };
    }
}

@TomasVotruba TomasVotruba requested a review from nikic September 26, 2022 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants